-
Notifications
You must be signed in to change notification settings - Fork 460
Make several TableOpsImpl methods atomic #5540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Make several TableOpsImpl methods atomic #5540
Conversation
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, code is also quite a bit more succinct now
} | ||
|
||
@Override | ||
public void clearSamplerConfiguration(String tableName) | ||
throws AccumuloException, TableNotFoundException, AccumuloSecurityException { | ||
throws AccumuloException, AccumuloSecurityException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this no longer throw TableNotFoundException? That seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing in this method throws TableNotFoundException
anymore I guess. In the old code, clearSamplerOptions()
threw it when it called getProperties()
but all of that has been replaced with the modifyProperties()
block which does not throw TableNotFoundException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect modifyProperties to throw TableNotFoundException. At least, some error should happen if the table doesn't exist... I'm not sure what error that is. But, I think this PR should wait until we figure that out, because this changes the behavior, and we shouldn't change the API behavior for the case when the table doesn't exist.
Fixes #5494
This PR replaces places were multiple calls to set or remove properties were being done with a single modify properties block to make things atomic. The affected methods are: